Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mvn spotless:apply now prints out statistics #1507

Merged
merged 8 commits into from
Jan 31, 2023
Merged

mvn spotless:apply now prints out statistics #1507

merged 8 commits into from
Jan 31, 2023

Conversation

blacelle
Copy link
Contributor

Follows #1506

@blacelle blacelle mentioned this pull request Jan 19, 2023
Closed
@nedtwigg
Copy link
Member

nedtwigg commented Jan 19, 2023 •
edited
Loading

I would categorize it like this: SpotlessJava is keeping 100 files clean - 10 were changed to be clean, 5 were already clean, 85 were skipped because caching determined they were already clean.

I think the PR as-is would be confusing because the first run would say that "100 files were considered" and after a few runs it would say "0 files were considered". The files were considered, a thorough up-to-date check showed that they were already clean :)

@blacelle
Copy link
Contributor Author

Here is another step forward. I introduce a Formatter.name property that I'm not 100% comfortable with.

The log now looks like:

[INFO] ------------------------------------------------------------------------
[INFO] Building Spotless Maven Plugin Tests 1.0.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- spotless-maven-plugin:2.31.0-SNAPSHOT:apply (default-cli) @ spotless-maven-plugin-tests ---
[INFO] Writing clean file: /private/var/folders/8b/p64c8tfs4d7gf3v8tcmwbz580000gn/T/junit17574313710916441137/json_test.json
[INFO] Spotless.Json is keeping 2 files clean - 1 were changed to be clean, 1 were already clean, 0 were skipped because caching determined they were already clean
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

I was keen to add the equivalent log in Gradle, but I'm too weak in Gradle. If this is a relevant improvment, I would appreciate a pointer to do so.

nedtwigg
nedtwigg requested changes Jan 30, 2023
View reviewed changes
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change, it's close to mergeable.

Re: the Gradle plugin, it's tough because the up-to-date checking happens within the gradle machinery, and I'm not aware of a way to get a count of how many were skipped due to up-to-date checking. Regardless, they're not coupled, so it can always be added later.

plugin-maven/src/main/java/com/diffplug/spotless/maven/ImpactedFilesTracker.java Outdated Show resolved Hide resolved
lib/src/main/java/com/diffplug/spotless/Formatter.java Outdated Show resolved Hide resolved
plugin-maven/src/main/java/com/diffplug/spotless/maven/ImpactedFilesTracker.java Outdated Show resolved Hide resolved
plugin-maven/src/main/java/com/diffplug/spotless/maven/SpotlessApplyMojo.java Outdated Show resolved Hide resolved
@blacelle blacelle marked this pull request as ready for review January 30, 2023 20:14
nedtwigg
nedtwigg approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Add an entry under ###Added in plugin-maven/CHANGELOG and I'll press merge.

@nedtwigg nedtwigg changed the title First commit to discuss the idea mvn spotless:apply now prints out statistics Jan 30, 2023
@blacelle
Copy link
Contributor Author

README entry added. I also added a condition so that synthesis kicks-in only if totalProcessed>0

@nedtwigg
Copy link
Member

I changed the debug to warn, as it's likely a user mistake to have a formatter which is doing nothing. Example such mistake

Are these changes okay with you? If so I will merge.

@blacelle
Copy link
Contributor Author

Thanks for the improvments.

@nedtwigg nedtwigg merged commit b290d8c into diffplug:main Jan 31, 2023
@blacelle blacelle deleted the AddLogSynthesysAfterApplyInMaven branch January 31, 2023 14:24
@blacelle
Copy link
Contributor Author

blacelle commented Jan 31, 2023 •
edited
Loading

@nedtwigg I'm not convinced (anymore) about the WARN in case of not a single matching file. As the maven-plugin is executed per module, we may typically have a <json> section in the parent multi-module, and have some sub-module not having a single json to format.

@nedtwigg
Copy link
Member

A fair amount of traffic on our bug tracker is

  • "it's not formatting any files"
  • "did you check the target includes"
  • "oh"

I'm happy to backtrack on this now or in the future, but it seems like it would be better to have a way for child pom to say e.g. <spotless><someFormat><disable>true to turn off warnings in that subproject.

@blacelle
Copy link
Contributor Author

@nedtwigg I'm also a user encountering many "it's not formatting any files". Which is one motivation for this PR. However, I foresee many WARNs in my own builds. I'd like to run a SNAPSHOT with one of my repository (to get a real feedback): do you know how I can deploy a SNAPSHOT in mvn local repository with the new setup ? (based on https://www.benediktritter.de/maven-plugin-development/)

@nedtwigg
Copy link
Member

nedtwigg commented Jan 31, 2023 •
edited
Loading

I think publishToMavenLocal should still work with the new maven-plugin-development build

But that's a good point, I think we can use JITPACK again now too. I just turned JITPACK building back on, and it looks like it's working okay as of current master 4c7b616e510bd83167ff329361ad819fac28ae6e

https://jitpack.io/com/github/diffplug/spotless/4c7b616e510bd83167ff329361ad819fac28ae6e/build.log

Might be broken, not 100% sure...

@nedtwigg
Copy link
Member

Ah, JITPACK is broken, working on that...

@nedtwigg
Copy link
Member

@blacelle
Copy link
Contributor Author

blacelle commented Jan 31, 2023 •
edited
Loading

./gradlew publishToMavenLocal

fails with:

spotless ./gradlew publishToMavenLocal

> Configure project :plugin-gradle
Signing plugin detected. Will automatically sign the published artifacts.

> Task :plugin-gradle:signPluginMavenPublication FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':plugin-gradle:signPluginMavenPublication'.
> Cannot perform signing task ':plugin-gradle:signPluginMavenPublication' because it has no configured signatory

* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.6/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 1s
41 actionable tasks: 9 executed, 32 up-to-date

Publishing build scan...
https://gradle.com/s/h4jvq7rusebeg

Same for ./gradlew maven-plugin:publishToMavenLocal

cd maven-plugin
../gradlew publishToMavenLocal

is OK.

@blacelle
Copy link
Contributor Author

A run of mvn com.diffplug.spotless:spotless-maven-plugin:2.32.0-SNAPSHOT:apply over https://github.com/solven-eu/pepper gives:

[INFO] --------------< io.github.solven-eu.pepper:pepper-parent >--------------
[INFO] Building pepper-parent 4.3-SNAPSHOT                               [1/25]
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- spotless-maven-plugin:2.32.0-SNAPSHOT:apply (default-cli) @ pepper-parent ---
[WARNING] Spotless.Format has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[WARNING] Spotless.Markdown has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[WARNING] Spotless.Json has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[WARNING] Spotless.Java has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[INFO] Sorting file /var/folders/8b/p64c8tfs4d7gf3v8tcmwbz580000gn/T/pom10821452448486918927.xml
[INFO] Pom file is already sorted, exiting
[INFO] Spotless.Pom is keeping 1 files clean - 0 were changed to be clean, 1 were already clean, 0 were skipped because caching determined they were already clean
[INFO] 
[INFO] --------------< io.github.solven-eu.pepper:pepper-static >--------------
[INFO] Building pepper-static 4.3-SNAPSHOT                               [2/25]
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- spotless-maven-plugin:2.32.0-SNAPSHOT:apply (default-cli) @ pepper-static ---
[WARNING] Spotless.Markdown has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[WARNING] Spotless.Format has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[INFO] Sorting file /var/folders/8b/p64c8tfs4d7gf3v8tcmwbz580000gn/T/pom9606408092992840722.xml
[INFO] Pom file is already sorted, exiting
[INFO] Spotless.Pom is keeping 1 files clean - 0 were changed to be clean, 1 were already clean, 0 were skipped because caching determined they were already clean
[WARNING] Spotless.Java has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[WARNING] Spotless.Json has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[INFO] 
[INFO] ------------< io.github.solven-eu.pepper:aggregator-pepper >------------
[INFO] Building aggregator-pepper 4.3-SNAPSHOT                           [3/25]
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- spotless-maven-plugin:2.32.0-SNAPSHOT:apply (default-cli) @ aggregator-pepper ---
[INFO] Sorting file /var/folders/8b/p64c8tfs4d7gf3v8tcmwbz580000gn/T/pom11573418929783516858.xml
[INFO] Pom file is already sorted, exiting
[INFO] Spotless.Pom is keeping 1 files clean - 0 were changed to be clean, 1 were already clean, 0 were skipped because caching determined they were already clean
[WARNING] Spotless.Java has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[INFO] Spotless.Json is keeping 5 files clean - 0 were changed to be clean, 5 were already clean, 0 were skipped because caching determined they were already clean
[INFO] Spotless.Markdown is keeping 3 files clean - 0 were changed to be clean, 3 were already clean, 0 were skipped because caching determined they were already clean
[WARNING] Spotless.Format has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[INFO] 
[INFO] --------------< io.github.solven-eu.pepper:pepper-agent >---------------
[INFO] Building pepper-agent 4.3-SNAPSHOT                                [4/25]
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- spotless-maven-plugin:2.32.0-SNAPSHOT:apply (default-cli) @ pepper-agent ---
[INFO] Spotless.Java is keeping 8 files clean - 0 were changed to be clean, 8 were already clean, 0 were skipped because caching determined they were already clean
[WARNING] Spotless.Json has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[INFO] Sorting file /var/folders/8b/p64c8tfs4d7gf3v8tcmwbz580000gn/T/pom16472238603268760438.xml
[INFO] Pom file is already sorted, exiting
[INFO] Spotless.Pom is keeping 1 files clean - 0 were changed to be clean, 1 were already clean, 0 were skipped because caching determined they were already clean
[WARNING] Spotless.Format has no target files. Examine your `<includes>`: https://github.com/diffplug/spotless/tree/main/plugin-maven#quickstart
[INFO] Spotless.Markdown is keeping 1 files clean - 0 were changed to be clean, 1 were already clean, 0 were skipped because caching determined they were already clean
...

This is many warnings in my perspective.

Regarding people with wrong includes: what's often lacking to me (with spotless) is proper loggings (e.g. this PR adds a DEBUG log when cleaning a file). I'm not sure (...) people with issues would try mvn ... -X, but as of now, even those digging into debug log does not get much to digest.

@nedtwigg
Copy link
Member

I am not familiar with Maven culture. Defining a bunch of goals in the parent pom to apply to children but don't have any effect seems like bad practice. But I'm not a maven user, and I'm happy to merge whatever native maven users feel the default behavior ought to be.

@blacelle
Copy link
Contributor Author

blacelle commented Feb 1, 2023

Defining a bunch of goals in the parent pom to apply to children but don't have any effect seems like bad practice.

In my perspective, this is totally legit and standard practise. But I'm just a single user. Do you know any other mvn Spotless user which may give his opinion on this?

@blacelle blacelle mentioned this pull request Feb 1, 2023
Merged
@nedtwigg
Copy link
Member

nedtwigg commented Feb 1, 2023

I always lean towards PR author's preference, merged :)

@nedtwigg
Copy link
Member

nedtwigg commented Feb 5, 2023

Published in 2.32.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nedtwigg nedtwigg nedtwigg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@blacelle @nedtwigg